Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make legend layouting respect padding at the start of columns #5776

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

jtagscherer
Copy link
Contributor

@jtagscherer jtagscherer commented Oct 19, 2018

The draw function of chart legends introduces a vertical padding at the very beginning of each column:

cursor = {
x: me.left + labelOpts.padding,
y: me.top + labelOpts.padding,
line: 0
};

Before individual items of a legend are being drawn, the fit function performs some layouting. This includes the calculation of how many columns of items will be needed to fit all items into a vertical legend. However, this calculation does not consider the aforementioned initial column padding. Instead, all new columns are assigned a height of 0:

var vPadding = labelOpts.padding;
var columnWidths = me.columnWidths = [];
var totalWidth = labelOpts.padding;
var currentColWidth = 0;
var currentColHeight = 0;
var itemHeight = fontSize + vPadding;
helpers.each(me.legendItems, function(legendItem, i) {
var boxWidth = getBoxWidth(labelOpts, fontSize);
var itemWidth = boxWidth + (fontSize / 2) + ctx.measureText(legendItem.text).width;
// If too tall, go to new column
if (currentColHeight + itemHeight > minSize.height) {
totalWidth += currentColWidth + labelOpts.padding;
columnWidths.push(currentColWidth); // previous column width
currentColWidth = 0;
currentColHeight = 0;
}
// Get max width
currentColWidth = Math.max(currentColWidth, itemWidth);
currentColHeight += itemHeight;
// Store the hitbox width and height here. Final position will be updated in `draw`
hitboxes[i] = {
left: 0,
top: 0,
width: itemWidth,
height: fontSize
};
});

In some edge cases with specific amounts of legend items and chart heights, this lead to overflow bugs as seen in #5491. This pull request fixes this issue.

Fixes #5491

etimberg
etimberg previously approved these changes Oct 19, 2018
@simonbrunel
Copy link
Member

simonbrunel commented Oct 20, 2018

I think I understand the issue but I'm not sure about the fix: currentColHeight should start from 0 because when there is no item, the height should be 0. However, it seems that we don't check currentColHeight against the correct layout item (maximum) height.

What about these changes:

var maxHeight = minSize.height - vPadding;

// ...

helpers.each(me.legendItems, function(legendItem, i) {
    // ...

    if (currentColHeight + itemHeight > maxHeight) {
       //...

Does that fix the issue?

@jtagscherer
Copy link
Contributor Author

Going into this direction would work as well. However, rather than adding the padding to the maxHeight, we would need to subtract it to account for the initial column padding. Therefore, the first line from your comment would need to be changed to:

var maxHeight = minSize.height - vPadding;

@simonbrunel Reverting my previous changes and going with this approach fixes this issue as well. What do you think? If you think we should go with this approach, I will commit the necessary changes.

@simonbrunel
Copy link
Member

@jtagscherer you right, I fixed my previous comment, it should be minSize.height - vPadding;

I would go with the approach I suggested. Do you confirm that it fixes #5491?

@jtagscherer
Copy link
Contributor Author

@simonbrunel Thank you for your suggestions, I have changed the code accordingly. Using a slightly adjusted version of the chart provided by the issue's author for testing, I can confirm that these changes fix #5491.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Oct 21, 2018
simonbrunel
simonbrunel previously approved these changes Oct 21, 2018
@simonbrunel
Copy link
Member

@jtagscherer can you try to add a unit test for this case to prevent regression?

@jtagscherer
Copy link
Contributor Author

@simonbrunel Sure thing! Should I add the unit test to this pull request or should I create a new one?

@simonbrunel
Copy link
Member

In this PR but you will need to wait a bit since GitHub have some issues.

@jtagscherer
Copy link
Contributor Author

Unfortunately, upon further testing and trying to create a minimal example for the unit test, this issue turned out to be of a larger scale than I expected. My changes did not fix the problem entirely. I'll try to outline it and offer some possibilities.

The draw function actually uses the legend's top and bottom attribute to decide whether a new column with legend items should be started. The fit function tries to get away with using the minSize for the same purpose, which does not work out in some cases. On top of that, the top and bottom attributes are different when accessed within the fit function. I strongly suspect that that is the case because of the function's position within the lifecycle, meaning that final chart layouting results are not available yet when fit is called.

Is there any reason why we have to calculate the amount of columns twice, once within fit and once within draw? Could we not do this just once within the draw function, since final layouting results are available here?

Alternatively, we could move the fit function to another position within the lifecycle and use top, bottom, left, and right attributes instead of the minSize. Doing so, we could guarantee that the column calculation results of both functions do not diverge. Calling fit from the afterLayout callback would work.

@simonbrunel What do you think? How should we go about this?

@simonbrunel
Copy link
Member

I'm really not familiar with the layout code, maybe @etimberg have more ideas on this issue?

@nagix
Copy link
Contributor

nagix commented Nov 7, 2018

I was originally trying to solve different legend layout issues in #5816, but realised that this PR is addressing a similar one. My solution to the fit/draw issue in that PR is to use minSize in the draw function. As recalculating the legend box every time the chart area size is changed will introduce more complexity, I just used the same values used in the fit function. The final result may not be perfect, but it is unnoticeable in most cases because of padding in the legend.

@jtagscherer
Copy link
Contributor Author

@nagix You are right, that seems to be the root cause for both of our issues, while my changes only addressed the symptoms. I tested the version of your pull request against my two test cases, and it fixed the issue in both of them. Therefore, I'd suggest that we go with your approach and fix this issue at its root.

Before that, it would be great if a maintainer that is familiar with the layout code could step in and explain why the current architecture with two diverging layout routines was chosen. Maybe there's a good reason for this, and our solution could cause even more problems.

Finally, I think it would be a good idea to add another test case against the specific issue that I was addressing. That way, we can ensure that there will be no regression. @nagix I already have a local finished test case that I could add. I could commit it if you give me write access to your pull request's forked repository, or I could create a pull request for your repository with my changes. Whatever works best for you.

@nagix
Copy link
Contributor

nagix commented Nov 9, 2018

I think the calculation in fit is needed because the height of the legend box is determined by the size of other boxes but the width can change depending on the legend items (in case of 'left' and 'right' positions). The problem is that the result of the fit function also affects the positions and sizes of other boxes, so we need to stop calculation at some point.

@jtagscherer Can you create a pull request for my repository? Then, I will merge it.

@simonbrunel
Copy link
Member

@jtagscherer @nagix can we close that PR in favor of #5816?

@jtagscherer
Copy link
Contributor Author

@simonbrunel Sorry for the delay. Yes, you can close this PR in favour of #5816. I would have added my unit test for the specific use case that the author of #5491 had an issue with to #5816 today, but I see that you have already merged it. That's not too bad though, since @nagix already added test cases for legends with multiple columns. We just won't have a regression test against that specific edge case in #5491.

@simonbrunel simonbrunel removed this from the Version 2.8 milestone Nov 12, 2018
@simonbrunel
Copy link
Member

@jtagscherer thanks for the update (and your contribution) :)

I totally forgot about the extra unit tests :\ sorry for that. @nagix, does your PR include regression tests for case described in #5491? If not and if it makes sense, @jtagscherer feel free to rebase and update this PR with these tests.

@jtagscherer
Copy link
Contributor Author

@simonbrunel No worries! I'll rebase this PR to include my regression tests covering the specific situation from #5491 and then we can decide whether they are worth the addition. I'd say that you can never have enough regression tests :)

@jtagscherer
Copy link
Contributor Author

@simonbrunel I have added a regression test for a simplified version of the issue in #5491. I have checked and can confirm that this test fails when run against the old version without #5816, but succeeds on the fixed version.

@nagix
Copy link
Contributor

nagix commented Nov 14, 2018

Thanks @jtagscherer, this looks good to me.

@nagix nagix self-requested a review November 14, 2018 00:34
@simonbrunel simonbrunel merged commit 3ea93a0 into chartjs:master Nov 14, 2018
@simonbrunel
Copy link
Member

Thanks @jtagscherer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Legend hides second column if has only one element
4 participants